Fix RandGridDistortiond crash when transform is skipped#8788
Fix RandGridDistortiond crash when transform is skipped#8788HeyangQin wants to merge 3 commits intoProject-MONAI:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMultiple dictionary-based spatial transforms were modified so they no longer call Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/transforms/spatial/dictionary.py (1)
2313-2315: Pre-existingconvert_to_tensor(d, ...)remains for edge case.When
first_key == ()(all keys missing), the entire dict is still converted. This matches other transforms (RandAffined,RandZoomd, etc.) but could cause issues with non-tensor metadata in that rare edge case. Consider addressing in a follow-up if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/transforms/spatial/dictionary.py` around lines 2313 - 2315, The branch handling the edge case where first_key == () currently calls convert_to_tensor(d, track_meta=get_track_meta()) which can inadvertently convert non-tensor metadata; update this branch to avoid converting the entire dict: either return the original dict unchanged or call convert_to_tensor only on entries that are tensor-compatible (filter keys by type or use an is_tensor_compatible check) and leave other metadata untouched. Modify the first_key == () logic in dictionary.py to use the safer behavior (referencing first_key, convert_to_tensor, and get_track_meta) so non-tensor metadata isn’t transformed unexpectedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@monai/transforms/spatial/dictionary.py`:
- Around line 2313-2315: The branch handling the edge case where first_key == ()
currently calls convert_to_tensor(d, track_meta=get_track_meta()) which can
inadvertently convert non-tensor metadata; update this branch to avoid
converting the entire dict: either return the original dict unchanged or call
convert_to_tensor only on entries that are tensor-compatible (filter keys by
type or use an is_tensor_compatible check) and leave other metadata untouched.
Modify the first_key == () logic in dictionary.py to use the safer behavior
(referencing first_key, convert_to_tensor, and get_track_meta) so non-tensor
metadata isn’t transformed unexpectedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44837c9e-8c0f-45ed-b0d1-06c2d7cac537
📒 Files selected for processing (2)
monai/transforms/spatial/dictionary.pytests/transforms/test_rand_grid_distortiond.py
When _do_transform is False, convert_to_tensor was called on the entire data dict, which fails when non-tensor values (e.g. ints, strings) are present — causing "AttributeError: 'int' object has no attribute 'numel'" in the DataLoader collate function. Convert only the keyed tensor items instead, consistent with how other dict transforms handle the no-transform case. Also return dict unchanged when no keys match (first_key == ()). Fixes Project-MONAI#8604 Signed-off-by: Heyang Qin <qysnn1@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/transforms/test_rand_grid_distortiond.py (1)
92-101: Consider documenting expected type conversion.Per
RandGridDistortiond.__call__(lines 2300-2303 in dictionary.py),convert_to_tensoris called on keyed items even when no transform is applied. Soresult["img"]becomes a tensor, not numpy. Thetype_test=Falsemasks this.Adding an explicit type check or comment would clarify this expected behavior.
📝 Suggested clarification
# non-tensor metadata should pass through unchanged self.assertEqual(result["extra_info"], 42) self.assertEqual(result["label_name"], "tumor") + # Note: convert_to_tensor is still called on keys, so img becomes tensor assert_allclose(result["img"], img, type_test=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transforms/test_rand_grid_distortiond.py` around lines 92 - 101, Update the test_no_transform_with_non_tensor_metadata test to assert the documented type conversion behavior: note that RandGridDistortiond.__call__ invokes convert_to_tensor on keyed items even when _do_transform is False, so "img" will be converted to a tensor; modify the assertions to either check for tensor type (e.g., isinstance(result["img"], torch.Tensor)) and compare values with an appropriate array/tensor comparison, or add a clarifying comment explaining why type_test=False is used and that result["img"] is expected to be a tensor due to convert_to_tensor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/transforms/test_rand_grid_distortiond.py`:
- Around line 92-101: Update the test_no_transform_with_non_tensor_metadata test
to assert the documented type conversion behavior: note that
RandGridDistortiond.__call__ invokes convert_to_tensor on keyed items even when
_do_transform is False, so "img" will be converted to a tensor; modify the
assertions to either check for tensor type (e.g., isinstance(result["img"],
torch.Tensor)) and compare values with an appropriate array/tensor comparison,
or add a clarifying comment explaining why type_test=False is used and that
result["img"] is expected to be a tensor due to convert_to_tensor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52024052-b702-43fc-9e97-978e58027360
📒 Files selected for processing (1)
tests/transforms/test_rand_grid_distortiond.py
HeyangQin
force-pushed
the
fix/rand-grid-distortiond-no-transform
branch
from
fe04817 to
7d5bf42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/transforms/spatial/dictionary.py`:
- Around line 2312-2314: The early-return behavior should be consistent: when
first_key(d) == () the transform should return the input dict unchanged rather
than calling convert_to_tensor and recursing; update the affected transforms
(e.g., RandAffined, Rand2DElasticd, Rand3DElasticd, RandZoomd,
RandSimulateLowResolutiond) to match RandAxisFlipd/RandGridDistortiond by
returning d immediately when first_key == (), removing the convert_to_tensor(d,
...) call in that branch; ensure you only change the branch that checks
first_key(d) and keep all other processing (including calls to convert_to_tensor
for non-empty keys) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ffaa117-392b-4bbd-baa0-176907781f7f
📒 Files selected for processing (2)
monai/transforms/spatial/dictionary.pytests/transforms/test_rand_grid_distortiond.py
✅ Files skipped from review due to trivial changes (1)
- tests/transforms/test_rand_grid_distortiond.py
…l transforms Unify the first_key == () handling in RandAffined, Rand2DElasticd, Rand3DElasticd, RandZoomd, and RandSimulateLowResolutiond to return the dict directly instead of calling convert_to_tensor on the entire dict, which could inadvertently convert non-tensor metadata. Signed-off-by: Heyang Qin <qysnn1@gmail.com>
Signed-off-by: Heyang Qin <qysnn1@gmail.com>
HeyangQin
force-pushed
the
fix/rand-grid-distortiond-no-transform
branch
from
fcc4cf9 to
bf00db8
Compare
1 participant
Summary
AttributeError: 'int' object has no attribute 'numel'whenRandGridDistortiondskips the transform (random probability not met)convert_to_tensor(d, ...)was called on the entire data dict, which fails when non-tensor metadata (ints, strings) is presentself.key_iterator(d), consistent with how other dict transforms handle the no-transform pathTest plan
extra_info=42,label_name="tumor") +prob=0.0passes without errorRandGridDistortiondandnum_workers>0no longer crashesFixes #8604